OpenSSL compatibility for libodbc#10813
Conversation
kojo1
changed the title
kojo1
force-pushed
the
odbc
branch
2 times, most recently
from
ad3c910 to
b41bbac
Compare
|
Retest please. |
dgarske
left a comment
dgarske
left a comment
There was a problem hiding this comment.
Skoll Code Review
Scan type: reviewOverall recommendation: REQUEST_CHANGES
Findings: 7 total — 7 posted, 0 skipped
5 finding(s) posted as inline comments (see file-level comments below)
2 finding(s) not tied to a diff line (full detail below)
Posted findings
- [High] OBJ_find_sigid_algs stub hardcodes SHA-256/RSA and ignores sigid —
wolfssl/openssl/objects.h:81-88 - [Medium] No test coverage for WOLFSSL_VERIFY_NONE_DEFAULT security-relevant default change —
src/internal.c:2645-2649 - [Low] WOLFSSL_ERR_R_BUF_LIB defined as 0 (conflicts with 'no error' and OpenSSL's value) —
wolfssl/openssl/err.h:46 - [Low] Garbled comment on OBJ_find_sigid_algs literal NIDs —
wolfssl/openssl/objects.h:77-79 - [Low] TLS_ST_OK hardcoded to 16, coupled to internal enum position —
wolfssl/openssl/ssl.h:1572
Findings not tied to a diff line
SSL_R_UNKNOWN_PROTOCOL / WRONG_VERSION_NUMBER / UNSUPPORTED_PROTOCOL numeric value changed
File: wolfssl/openssl/ssl.h:1748-1753, wolfssl/openssl/err.h:47-49
Function: N/A (macros)
Severity: Medium
These three pre-existing public compat macros were changed from expanding to VERSION_ERROR (a negative wolfSSL internal error enum) to OpenSSL numeric reason codes (WOLFSSL_SSL_R_UNKNOWN_PROTOCOL=252, WOLFSSL_SSL_R_WRONG_VERSION_NUMBER=267, WOLFSSL_SSL_R_UNSUPPORTED_PROTOCOL=258). This is more OpenSSL-accurate, but it is a behavior change: any existing application that compared these macros against wolfSSL's internally reported error code (VERSION_ERROR, e.g. from wolfSSL_get_error/SSL_get_error) will no longer match, because wolfSSL still reports VERSION_ERROR internally, not 252/267/258. I confirmed there are no internal src/*.c or tests/*.c users of these specific macros (tests compare against VERSION_ERROR directly), so the build and current tests are unaffected, but downstream consumers relying on the old mapping would silently break.
Recommendation: Confirm this value change is intended and note it as a compatibility change in the PR/changelog. If wolfSSL's error surface (ERR_get_error/ERR_GET_REASON) does not translate internal VERSION_ERROR into these OpenSSL reason codes, apps checking reason == SSL_R_WRONG_VERSION_NUMBER will never match - verify the intended consumer (libodbc/libtds) actually reads these as raw constants and not via wolfSSL's error queue.
Referenced code: wolfssl/openssl/ssl.h:1748-1753, wolfssl/openssl/err.h:47-50 (4 lines)
BIO compat stubs have latent multi-use hazards (fixed index, app_data aliasing)
File: wolfssl/openssl/bio.h:196, wolfssl/openssl/ssl.h:1771-1774
Function: wolfSSL_BIO_get_new_index
Severity: Medium
Two BIO compat shims work for a single custom BIO but are unsafe if used more than once: (1) wolfSSL_BIO_get_new_index() always returns the constant 1000 - in OpenSSL each call returns a distinct index, so an app that registers two custom BIO types would get identical type values and collide. (2) BIO_get_app_data/BIO_set_app_data are mapped onto wolfSSL_BIO_get_data/wolfSSL_BIO_set_data, which is the SAME storage slot that a custom BIO method already uses for its method-private data. In OpenSSL these are two independent slots; collapsing them means an app that uses both BIO_set_data (method state) and BIO_set_app_data (app state) on the same BIO will overwrite one with the other. The wolfSSL_BIO_meth_get_* getters returning NULL and set_callback_ctrl being a no-op are acceptable documented stubs.
Recommendation: Keep for the narrow libodbc/libtds use, but document the single-custom-BIO limitation in the header comment. If feasible, back get_new_index with a static incrementing counter, and keep app_data in a separate slot from method data to avoid silent corruption when both are used.
Referenced code: wolfssl/openssl/bio.h:196, wolfssl/openssl/ssl.h:1771-1774 (4 lines)
Review generated by Skoll
| * RSA-with-SHA-256 channel binding. Literal NIDs (672, 6) is for | ||
| * ASN is disabled. */ | ||
| #ifndef BUILDING_WOLFSSL | ||
| static WC_INLINE int |
There was a problem hiding this comment.
🔴 [High] OBJ_find_sigid_algs stub hardcodes SHA-256/RSA and ignores sigid
The new wolfSSL_OBJ_find_sigid_algs() casts away its sigid input ((void)sigid;) and unconditionally reports *pdig = 672 (NID_sha256) and *ppkey = 6 (NID_rsaEncryption) for ALL signature algorithms, then returns success (1). This is an always-success stub returning fixed values regardless of input, which the wolfSSL C coding conventions explicitly forbid ("No always-success stubs - return a 'not implemented' error instead"). Functionally it is wrong for any certificate not signed with sha256WithRSAEncryption: for an ECDSA cert or an RSA cert signed with SHA-384/SHA-512, this reports SHA-256+RSA. libpq's tls-server-end-point channel binding derives the binding hash from the server certificate's signature algorithm (via X509_get_signature_nid() -> OBJ_find_sigid_algs()); reporting the wrong digest/pkey will compute the wrong channel-binding hash and cause the bound connection to fail against a real peer, or silently produce a non-interoperable binding. The narrow libpq use case (RSA+SHA-256) works, but the function is a latent correctness/interop defect for the general case and gives no way for the caller to detect the substitution.
Fix: Do not ship an always-success stub that returns fixed crypto parameters. Either map the real signature algorithm, or return 0 for anything other than the algorithm(s) actually supported so callers can detect the unsupported case.
|
|
||
| XMEMSET(ctx, 0, sizeof(WOLFSSL_CTX)); | ||
|
|
||
| #ifdef WOLFSSL_VERIFY_NONE_DEFAULT |
There was a problem hiding this comment.
🟠 [Medium] No test coverage for WOLFSSL_VERIFY_NONE_DEFAULT security-relevant default change
The new WOLFSSL_VERIFY_NONE_DEFAULT build option flips the default peer-verification behavior of a freshly created CTX (and every SSL it spawns, via ssl->options.verifyNone = ctx->verifyNone) from verify to no-verify. This is a security-relevant default and there is no test asserting (a) that with the macro set a new CTX has verifyNone and an SSL inherits it, and (b) that a subsequent wolfSSL_CTX_set_verify(ctx, WOLFSSL_VERIFY_PEER, ...) correctly clears the default. The PR checklist 'added tests' is unchecked.
Fix: Add an API test (guarded on WOLFSSL_VERIFY_NONE_DEFAULT) that creates a CTX, checks the default is VERIFY_NONE, verifies an SSL inherits it, and verifies wolfSSL_CTX_set_verify(WOLFSSL_VERIFY_PEER) overrides it. Also confirm interaction with wolfSSL_CTX_set_verify(WOLFSSL_VERIFY_DEFAULT), which resets verifyNone back to 0.
| /* reasons */ | ||
| #define WOLFSSL_ERR_R_SYS_LIB 1 | ||
| #define WOLFSSL_PKCS12_R_MAC_VERIFY_FAILURE 2 | ||
| #define WOLFSSL_ERR_R_BUF_LIB 0 |
There was a problem hiding this comment.
🔵 [Low] WOLFSSL_ERR_R_BUF_LIB defined as 0 (conflicts with 'no error' and OpenSSL's value)
WOLFSSL_ERR_R_BUF_LIB (mapped to ERR_R_BUF_LIB) is defined as 0. An error reason value of 0 is conventionally 'no error/unset', and 0 is also used as the function-code sentinel in this file's wolfSSL_SSLerr(0,...) macros, so an app doing if (ERR_GET_REASON(e) == ERR_R_BUF_LIB) could match on a cleared/zero reason. It also does not match OpenSSL, where ERR_R_BUF_LIB is a nonzero library reason code. The other reasons in this block are nonzero (SYS_LIB=1, MAC_VERIFY_FAILURE=2).
Fix: Give ERR_R_BUF_LIB a distinct nonzero value (matching OpenSSL's convention if the consumer compares raw values), or confirm the consumer never treats 0 as a meaningful reason.
| #define NID_ad_OCSP WC_NID_ad_OCSP | ||
| #define NID_ad_ca_issuers WC_NID_ad_ca_issuers | ||
|
|
||
| /* OBJ_find_sigid_algs(): report SHA-256 / RSA for libpq's |
There was a problem hiding this comment.
🔵 [Low] Garbled comment on OBJ_find_sigid_algs literal NIDs
The comment 'Literal NIDs (672, 6) is for ASN is disabled.' is grammatically broken and unclear (double 'is'). It appears to intend 'Literal NIDs (672, 6) are used in case ASN is disabled.'
Fix: Fix the comment wording.
| #define SSL_get_state wolfSSL_get_state | ||
| #define SSL_state_string_long wolfSSL_state_string_long | ||
|
|
||
| #define WOLFSSL_TLS_ST_OK 16 |
There was a problem hiding this comment.
🔵 [Low] TLS_ST_OK hardcoded to 16, coupled to internal enum position
WOLFSSL_TLS_ST_OK is defined as the literal 16. This is correct today because wolfSSL_get_state() returns ssl->options.handShakeState, and HANDSHAKE_DONE is the 17th member (index 16) of the internal enum states. However, 16 is a magic number duplicating an internal enum value: if any state is ever inserted before HANDSHAKE_DONE, this constant silently becomes wrong. The internal enum already carries a warning comment ('Adding state before HANDSHAKE_DONE will break session importing'), so risk is low, but the coupling is not obvious from the public header.
Fix: Add a comment noting that 16 must equal the internal HANDSHAKE_DONE enum value (wolfssl/internal.h enum states), so a future editor keeps them in sync.
3 participants
Description
OpenSSL compatibility for libodbc.
Fixes zd#21884
Testing
Integration test with:
[ODBC application]
↓ unixODBC (libodbc.2)
├─→ psqlodbcw.so ─→ libpq.5.dylib ─┐
└─→ libtdsodbc.so ─────────┴─→ libwolfssl.44.dylib
Checklist